Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: handle cancelling requests #194

Merged
merged 4 commits into from
Oct 24, 2023
Merged

feat: handle cancelling requests #194

merged 4 commits into from
Oct 24, 2023

Conversation

abc3
Copy link
Member

@abc3 abc3 commented Oct 20, 2023

This PR adds support for query cancellation, allowing clients to terminate long-running queries. Additionally, this PR includes the forwarding of the db name from the client connection to pool initialization.

Links for context:

@abc3 abc3 requested review from chasers and a team October 20, 2023 13:08
@@ -110,6 +110,13 @@ defmodule Supavisor.DbHandler do
%{tag: :ready_for_query, payload: db_state}, {ps, _} ->
{ps, db_state}

%{tag: :backend_key_data, payload: payload}, acc ->
key = {data.tenant, self()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may not need to have self() as part of the key. Instead you can just use data.tenant and use Registry.values to look up the values for a given key-pid pair.

@josevalim
Copy link
Contributor

The linked Postgres docs are helpful but I am struggling a bit to understand the BackendKeyData bits. When you connect, do you send the BackendKeyData to Postgres or do you receive it from them?

@abc3
Copy link
Member Author

abc3 commented Oct 23, 2023

@josevalim Sorry for the lack of explanation.

DB clients receive BackendKeyData, which identifies a unique connection between the client and the database. We need to store these keys which are received by the pool processes (db_handler). Additionally, we need to send our unique keys to clients connected to Supavisor. When clients request a cancel query, we send this event to the client_handler. The client_handler then sends it to the tenant's database using the key from the associated pool process

@josevalim
Copy link
Contributor

I see. We received it from the database but, because it is a proxy, we need to generate ours too, but so far the ones we generate are not used for anything?

@abc3
Copy link
Member Author

abc3 commented Oct 23, 2023

They are used. When the client_handler sends BackendKeyData, it then subscribes to this pid/key pair. If the client sends a cancellation request in another connection, we will receive it inside existing client_handler process

@josevalim
Copy link
Contributor

Perfect, thank you! :shipit:

@abc3
Copy link
Member Author

abc3 commented Oct 23, 2023

let me know if these chains are too complicated and if there's a way to make them more straightforward 🙏

@josevalim
Copy link
Contributor

The complexity is not code-wise but the fact it is scattered across different modules. But it has to be scattered, because it comes from places with different responsibilities. Something that could help is to have some mermaid.js diagrams (like we do here). They are supported by GitHub too. However, it is completely your decision if it is worth the time investment. Don't do it on my account. :)

@abc3 abc3 merged commit c6e7335 into main Oct 24, 2023
2 checks passed
@abc3 abc3 deleted the feat/cancel-cmd branch October 24, 2023 08:14
abc3 added a commit that referenced this pull request Oct 26, 2023
* chore: move docs to mkdocs (#186)
* docs: auto publish docs on main (#189)
* fix: install mkdocs-material (#192)
* docs: more docs (#190)
* feat: add gh workflow (#180)
* fix: handle http request in the native mode (#193)
* feat: handle cancelling requests (#194)
* fix: prevent client <-> db locking (#195)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants